Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

275 add co2 sensor support to smibhid #276

Merged
merged 16 commits into from
Mar 16, 2025

Conversation

sjefferson99
Copy link
Member

Add capability to add sensor modules on I2C that will poll on async loop and present their module information and readings on the API.
Fixed I2C not being defined at HID and main level to prevent incorrectly specifying different I2C parameters across the code.
Fixed updater crashing if the updates folder is not present.

@sjefferson99 sjefferson99 requested a review from sam57719 March 7, 2025 23:49
@sjefferson99 sjefferson99 linked an issue Mar 7, 2025 that may be closed by this pull request
@sjefferson99
Copy link
Member Author

Added BME280 module for easier testing.

@sjefferson99 sjefferson99 linked an issue Mar 12, 2025 that may be closed by this pull request
@sam57719
Copy link
Collaborator

Just tried some tests without any modules installed as I don't have any yet.
Generally looks good, however I was expecting a 404 not found error rather than a 500 internal server error for trying to access a sensor or its readings that doesn't exist

[2021-1-1 0:0:16][Mem: 39kB free][Error][Sensors]: Failed to load SGP30 sensor module: [Errno 5] EIO
[2021-1-1 0:0:16][Mem: 37kB free][Error][Sensors]: Failed to load BME280 sensor module: [Errno 5] EIO
[2021-1-1 0:0:16][Mem: 34kB free][Error][Sensors]: Failed to load SCD30 sensor module: 
[2025-3-12 20:48:49][Mem: 65kB free][Error][TinyWeb]: /api/sensors/%7Bmodule%7D
[2025-3-12 20:48:49][Mem: 63kB free][Error][TinyWeb]: Unhandled exception in user's method. Original error: %7Bmodule%7D
[2025-3-12 20:48:54][Mem: 65kB free][Error][TinyWeb]: /api/sensors/
[2025-3-12 20:48:54][Mem: 63kB free][Error][TinyWeb]: Unhandled exception in user's method. Original error: function missing required positional argument #2
[2025-3-12 20:48:59][Mem: 65kB free][Error][TinyWeb]: /api/sensors/readings/%7Bmodule%7D
[2025-3-12 20:48:59][Mem: 63kB free][Error][TinyWeb]: Unhandled exception in user's method. Original error: %7Bmodule%7D
[2025-3-12 20:49:43][Mem: 66kB free][Error][TinyWeb]: /api/sensors/123
[2025-3-12 20:49:44][Mem: 64kB free][Error][TinyWeb]: Unhandled exception in user's method. Original error: 123
[2025-3-12 20:50:23][Mem: 65kB free][Error][TinyWeb]: /api/sensors/crap-sensor
[2025-3-12 20:50:23][Mem: 63kB free][Error][TinyWeb]: Unhandled exception in user's method. Original error: crap-sensor

@sjefferson99
Copy link
Member Author

Not great at tinyweb/flask so have yet to really look at that, is a valid finding. But we could choose to add to a new issue to improve rather than adding complexity to this one given how big it has already grown. Any ideas how to modify the website.py file to achieve this?

@sam57719
Copy link
Collaborator

Personally I would suggest a layout more like this, grouping things better? You may disagree

Method Endpoint Description
GET /api/sensors/modules List available sensor modules
GET /api/sensors/modules/{module} List sensors in a specific module
GET /api/sensors/modules/{module}/readings/latest Get latest readings from a specific module
GET /api/sensors/readings/latest Get latest sensor readings from all modules

@sam57719
Copy link
Collaborator

R.e. the 404 thing

looks like if you do the following a 404 error is returned correctly. I tested it with a broad try/except but the KeyError looks to be the exception that gets triggered.

from http.webserver import HTTPException
...
...
...
class Modules():

    def get(self, data, sensors, logger: uLogger) -> str:
        logger.info("API request - sensors/modules")
        try:
            html = dumps(sensors.get_modules())
            logger.info(f"Return value: {html}")
        except KeyError:
            raise HTTPException(404)
        return html

@sam57719
Copy link
Collaborator

Have you had a chance to look at the 404 thing from the above comment?

Do we want as a different issue?

@sam57719
Copy link
Collaborator

Just tried the BME280 module, am investigating

Task exception wasn't retrieved
future: <Task> coro= <generator object '_poll_sensors' at 20024260>
Traceback (most recent call last):
  File "asyncio/core.py", line 1, in run_until_complete
  File "lib/sensors/__init__.py", line 70, in _poll_sensors
  File "lib/sensors/__init__.py", line 98, in get_readings
  File "lib/sensors/BME280.py", line 263, in get_reading
  File "lib/sensors/BME280.py", line 176, in read_compensated_data
  File "lib/sensors/BME280.py", line 161, in read_raw_data
TypeError: can't convert float to int

@sjefferson99
Copy link
Member Author

sjefferson99 commented Mar 14, 2025 via email

@sam57719
Copy link
Collaborator

sam57719 commented Mar 14, 2025

BME280 looks good with that int() fix, not sure if its my sensor, but I am reading 110000.0 hPa of pressure - shouldnt it be more like 1100?
Other readings look normal

{
  "BME280": {
    "pressure": 110000,
    "humidity": 41.1409,
    "temp": 17.31532
  }
}

@sjefferson99
Copy link
Member Author

In my haste to get it working, I missed that the library had a property that formatted the raw values correctly, I have called that in get_reading() instead and it all works much better. Pushed that to branch. I might make a similar version that adds the units in the dict for better consumption by a script calling the API. But will do that when needed.

…units and adjust sensor list to list of dicts including bame and unit.
@sjefferson99
Copy link
Member Author

Adjusted all 3 drivers to provide sensible precision for values and remove all units. Adjusted list of sensors to be a list of dicts with name and unit.

@sjefferson99
Copy link
Member Author

Personally I would suggest a layout more like this, grouping things better? You may disagree

Method Endpoint Description
GET /api/sensors/modules List available sensor modules
GET /api/sensors/modules/{module} List sensors in a specific module
GET /api/sensors/modules/{module}/readings/latest Get latest readings from a specific module
GET /api/sensors/readings/latest Get latest sensor readings from all modules

This is logical, however tinyweb only seems to support the variable being the URL suffix and not mid URL. Will see if this can be fixed easily.

@sjefferson99
Copy link
Member Author

I'm going to restructure as suggested but not provide the module specific reading list for now. It looks like tinyweb has a PR that would solve this, but is not something I will put in this PR as a little complex. belyalov/tinyweb#51

@sam57719
Copy link
Collaborator

Looks good, apart from the /api/sensors/modules/BME280 doesnt return valid JSON, its missing the closing ]

[
  {
    "name": "pressure",
    "unit": "hPa"
  },
  {
    "name": "temperature",
    "unit": "°C"
  },
  {
    "name": "humidity",
    "unit": "%"
  }

@sjefferson99
Copy link
Member Author

Degrees sign was messing with JSON dumps() removed and added comment not to readd. I suspect going down a fix dumps upstream for special characters rabbit hole is not a good use of time.

Copy link
Collaborator

@sam57719 sam57719 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All issues I found have been sorted!

@sjefferson99 sjefferson99 merged commit 3e51667 into master Mar 16, 2025
@sjefferson99 sjefferson99 deleted the 275-add-co2-sensor-support-to-smibhid branch March 16, 2025 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SCD-30 CO2 sensor support to SMIBHID Add CO2 sensor support to SMIBHID
2 participants